- 
                Notifications
    You must be signed in to change notification settings 
- Fork 65
ci: use submodules for freetype #1585
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Engine or game submodules maybe much recent than the one the system provides, and define symbols not found in system libraries. While it is probably unlikely to happen with freetype, this is more likely to happen with some game libraries like Lua.
91d83d6    to
    6d3f0a1      
    Compare
  
    | endif() | ||
|  | ||
| option(PREFER_EXTERNAL_LIBS "Tries to use system libs where possible." ON) | ||
| option(PREFER_EXTERNAL_LIBS "Tries to use system libs where possible." OFF) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be changed because Unvanquished doesn't even build in the default configuration that way (it defines the Freetype lib twice).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Freetype submodule double build bug is now fixed.
When building both the engine and a native game, the native game now reuses the Freetype target already sets by the engine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Freetype submodule double build bug is now fixed.
Great!
What's the motivation for the change though? It makes builds longer and people building Daemon themselves can no longer benefit from distro security updates.
… Freetype submodule
Some libraries may be built statically before being linked against game dll, so -fPIC would be required, and then we better compile everything with -fPIC.
6d3f0a1    to
    a73200a      
    Compare
  
    8b508fb    to
    f5e9697      
    Compare
  
            
          
                CMakeLists.txt
              
                Outdated
          
        
      | if (PREFER_EXTERNAL_LIBS AND NOT NACL) | ||
| find_package(${LIB_NAME}) | ||
| if (NOT ${LIB_NAME}_FOUND) | ||
| if (PREFER_EXTERNAL_LIBS AND NOT NACL) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rest of this file is using spaces, not tabs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
| endif() | ||
|  | ||
| option(PREFER_EXTERNAL_LIBS "Tries to use system libs where possible." ON) | ||
| option(PREFER_EXTERNAL_LIBS "Tries to use system libs where possible." OFF) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Freetype submodule double build bug is now fixed.
Great!
What's the motivation for the change though? It makes builds longer and people building Daemon themselves can no longer benefit from distro security updates.
| 
 The motivation is that we better better strictly control what we build against when doing our own official release builds. We already rebuild every deps using the  And when a library is used in both game and engine, like freetype, we better use the exact same freetype library in engine so we don't deal with different bugs and have consistent version to track down. Then we better build stuff in our CI the closest way we build the release, building the submodule in both cases is the easiest way to achieve that. Note that for non-Linux systems where we usually ship many prebuilt libraries in external deps archives, we may modify the external_deps script to build from submodule so we ensure prebuilt libraries are exactly the same as submodule ones. This can be implemented above #1433: 
 This is not true at all. The whole reason why  If this prevent to benefit from distro security updates or just simply rely on system libraries, the whole  | 
f5e9697    to
    5ee903c      
    Compare
  
    | SGTM to use the submodule in the release script and to make the CI match what is done with the release. I'm just disputing what should be the default value. To benefit players who build their own engine from source, the default should be to use the system libs. Release and CI can customize everything so the default doesn't matter for them. | 
5ee903c    to
    23bc0d2      
    Compare
  
    | I extracted the non “default switching” changes there and there: 
 We better merge them in all case. I'm myself not entirely convinced about what's the best to do for the default, but we can already fix the existing bugs without deciding on the default. | 
Our submodules may provide symbols not provided by system libraries.
Makes sure libraries that may be used by both the engine and the game are built once.
See on game side: